Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: Remove PropsTable, clean ArgsTable stories #11105

Merged
merged 7 commits into from
Jun 10, 2020

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 9, 2020

Issue: N/A

What I did

This is pre-work for an ArgsTable redesign

How to test

CI is passing

@shilman shilman changed the title ArgsTable: Clean up related stories ArgsTable: Clean up related stories (WIP) Jun 9, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2020

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against 51e94df

@shilman shilman requested a review from stijnkoopal as a code owner June 9, 2020 23:25
@shilman shilman added addon: docs maintenance User-facing maintenance tasks BREAKING CHANGE labels Jun 10, 2020
@shilman shilman changed the title ArgsTable: Clean up related stories (WIP) Components: Remove PropsTable, clean ArgsTable stories Jun 10, 2020
@shilman shilman removed the maintenance User-facing maintenance tasks label Jun 10, 2020
@shilman shilman added this to the 6.0 docs milestone Jun 10, 2020
@shilman
Copy link
Member Author

shilman commented Jun 10, 2020

@tmeasday this is some pre-work for @domyen 's updated ArgsTable design. Can you quickly review the changes in *.stories.tsx before I move on to the main event? If you're good with the changes, I will do a blanket update to the storybook repo with this kind of stuff, as part of the 6.0 args push.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stories look great a couple suggestions

lib/components/src/blocks/ArgsTable/ArgsTable.stories.tsx Outdated Show resolved Hide resolved
{propsTableStories.normal()}
{sourceStories.jsx()}
</>
<Description.Text {...Description.Text.args} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should do the default parameter binding trick for these stories?

Copy link
Member Author

@shilman shilman Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default parameter binding trick doesn't seem to work. <Description.Text /> seems to fill in props with {}, overriding the trick

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I see what you mean, I guess react passes {} by default not undefined which breaks that pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😞

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for react it's really Story.defaultProps = Story.args method the only way?

@shilman shilman merged commit 6aaa9d4 into next Jun 10, 2020
@ndelangen ndelangen deleted the 10979-argstable-design-update branch June 10, 2020 11:34
@Bruno-Tavares-Bose
Copy link

Bruno-Tavares-Bose commented Jun 15, 2020

I know that this is a beta version and this PR is marked with BREAKING CHANGE, but, since this (beta.23 is ok), I can not see props in our React components. We are using MDX (docs addon), and our Vue components have their props in there. Is there any readme, to understand the breaking changes? And why the Vue components are ok, but not the React ones?

Thanks in advance.

[edited] I can not see

@shilman
Copy link
Member Author

shilman commented Jun 16, 2020

We try to document all breaking changes here: https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#from-version-53x-to-60x

This PR in particular is a breaking change because I removed a component from @storybook/components, so any code that used that component would break. I'm pretty sure only Storybook used that component internally, but since it's a public API I don't know.

@Bruno-Tavares-Bose
Copy link

Thanks for the fast reply.

From the docs and code, I don't get why we are not seeing our props in React components.

We have one storybook for both React and Vue components, and we are doing these:

import { extractArgTypes as reactExtractArgTypes } from '@storybook/addon-docs/dist/frameworks/react/extractArgTypes';
import { extractArgTypes as vueExtractArgTypes } from '@storybook/addon-docs/dist/frameworks/vue/extractArgTypes';
import { createElement } from 'react';
import toReact from '@egoist/vue-to-react';

export const parameters = {
	docs: {
		extractArgTypes(component) { // would be awesome to have access to context like we have in decorators 😉
			if (typeof component === 'function') {
				return reactExtractArgTypes(component);
			}

			return vueExtractArgTypes(component);
		}
	}
};

export const decorators = [
	(storyFn, { kind }) => {
		let story = storyFn();

		const [framework] = kind.split('/');
		switch (framework) {
			case 'vue':
				const Story = toReact(story);
				story = createElement(Story, null);
				break;
		}

		return story;
	}
];

This have been working till beta.23, but after beta.24 we are getting the Vue as normal, but not the React. I already tried to comment the extractArgTypes method, and we get the Vue props (but the react table [without props title row for example]), but never the React.
We are using React functions components with hooks (no typescript).

Putting a console.log in estractArgTypes, it seems that the React component don't have the __docgenInfo. For Vue to work with React we also add the:

module: {
	rules: [
		{
			test: /\.vue$/,
			loader: 'vue-docgen-loader',
			enforce: 'post',
			options: {
				docgenOptions: Object.assign(
					{
						alias: config.resolve.alias
					},
					options.vueDocgenOptions,
					{
						alias: resolve.alias
					}
				)
			}
		}
	]
}

in main.js, should we have to use the a react docgen loader? And how it would be?

Again, thanks in advance

@shilman
Copy link
Member Author

shilman commented Jun 16, 2020

@Bruno-Tavares-Bose

First off, the way you are using Storybook is definitely not supported. We are releasing a new feature called composition in SB 6.0, that should allow you to mix frameworks more cleanly.

That said, the big change in beta.24 that's affecting your props generation is probably this one: #11106

There's one open issue so far that might be related: #11146

And there's an open PR that attempts to fix it: #11184

Hope that helps! cc @hipstersmoothie

@Bruno-Tavares-Bose
Copy link

Exploring a bit this issue, I also ended up in #11106 . Forcing the babel-plugin-react-docgen (bypassing this) fix the props (we are receiving some warnings now the types mismatch, picking).
And passing this typescript: { reactDocgen: 'react-docgen' } in main.js have the same result.

I will follow that PR. Thanks for the help, and continue with the awesome work 🙇

@hipstersmoothie
Copy link
Contributor

We are using React functions components with hooks (no typescript).

This stands out to me. I don' t think storybook should be using react-docgen-typescript-plugin when it isn't typescript. react-docgen-typescript works by doing the following:

  1. loads code into typescript
  2. extracts docgen info from the compiler

If a user is using prop-types instead of interfaces the docgen from react-docgen-typescript-plugin will produce nothing.

@hipstersmoothie
Copy link
Contributor

hipstersmoothie commented Jun 16, 2020

Thinking about this I should probably remove this check https://github.com/storybookjs/storybook/blob/next/app/react/src/server/framework-preset-react-docgen.ts#L9-L11

so that babel-docgen is always loaded. Since the default is react-docgen-typescript a JS-only user would always have to set reactDocgen to react-docgen.

@shilman @mrmckeb I feel like there should be some way to detect if we want the babel-plugin-react-docgen plugin to be loaded rather than always defaulting to typescript

EDIT: but then there isn't a guarantee that there isn't a mix of JS and TS components...

@Bruno-Tavares-Bose
Copy link

Hi @hipstersmoothie I think that is exactly the issue.

And sorry to spam this PR 😊

@hipstersmoothie
Copy link
Contributor

hipstersmoothie commented Jun 16, 2020

It's kinda bad but I think storybook will have to do the docgen parsing twice because there is no way we can detect if a project is:

  • pure JS
  • pure TS
  • mixed JS/TS

@hipstersmoothie
Copy link
Contributor

It might make more sense for react-docgen to be the default and users can opt-in to react-docgen-typescript if they want more detailed prop docs.

@shilman thoughts?

@shilman
Copy link
Member Author

shilman commented Jun 17, 2020

@hipstersmoothie we tried making react-docgen the default already (for performance reasons) but its typescript handling is just not good enough yet. I don’t understand the issue? Why can’t react-docgen-typescript ONLY apply to .ts/.tsx files? I think we had that setup working before with react-docgen-typescript-loader cc @mrmckeb

@hipstersmoothie
Copy link
Contributor

The problem is this line which makes it so that 'babel-plugin-react-docgen' is only loaded when it is set to use that.

This means that JS only users do not have 'babel-plugin-react-docgen' loaded because the default is react-docgen-typescript.

I changed that line because I didn't want 2 docgen to run on my components: 1 by babel and 1 by the plugin.

@shilman
Copy link
Member Author

shilman commented Jun 17, 2020

what about this line:

https://github.com/storybookjs/storybook/blob/next/app/react/src/server/framework-preset-react-docgen.ts#L17

doesn't it skip tsx? files if user is running react-docgen-typescript? i think the line you mention is just a bug.

@hipstersmoothie
Copy link
Contributor

Totally missed that. You’re right!

@Bruno-Tavares-Bose
Copy link

Great discussion guys. Just tried the new release (beta.31) and we have back our props in React components. 👍 💪 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants